Replace std.log with a structured logger #691
Conversation
sjorsdonkers
left a comment
sjorsdonkers
left a comment
There was a problem hiding this comment.
Pierre found some log related performance regression in a February commit.
Is this PR in response to that?
Does this PR have performance impact?
src/log.zig
Outdated
| const Pool = struct { | ||
| loggers: []*Log, | ||
| available: usize, | ||
| mutex: Thread.Mutex, | ||
| cond: Thread.Condition, |
There was a problem hiding this comment.
It is not clear to me why we have a Log Pool as opposed to a thread local logger. So 1 per thread.
Specifically since we are also locking the Out before writing.
I understand it may save a bit of memory if we have many threads, but does introduce some complex performance dependency. It feels is a bit premature.
There was a problem hiding this comment.
Well, initially the logging had more advanced use-cases that would result in more than 1 logger active per thread.
I'm ok with starting with TLS, but I'm not sure how I'd initialize the instance given it needs an allocator.
I didn't know about the performance thing, but I doubt this would be any faster. It adds a bit more formatting, i.e. escaping strings. This is really about adding structured logging to make it easier to parse the output programmatically. I'd also like to make the log-level a runtime configuration (i.e. ./lightpanda --log_level debug) rather than a compile-time one. And that's a bit messy to do with std.log...although that'll also add performance overhead. |
Outputs in logfmt in release and a "pretty" print in debug mode. The format along with the log level will become arguments to the binary at some point in the future.
Trying to achieve a few different things:
1 - In a deployed / production environment, using a standard format (logfmt) so that the logs can easily be ingested by tools like vector or logstash, etc..
2 - In a development environment, printing easier to read logs (subjective)
3 - Generally having more standardized logs. Imagine an HTTP fetch fails. With Zig's format-based logging, you have something like:
log.err("request {s} failed: {}", .{url, err});. It works, but it isn't particularly parser friendly. Plus, it can be hard to keep consistent with other logs. For example:log.info("fetching: '{s}'", .{url});Now the URL is in quotes and it's after the colon. With the structured logs, theurlanderrare arguments:log.err(.http_client, "request failed", .{.url = url, .err = err}); which, inlogfmt` generates key=value attribute.The pretty output isn't that special, but my old eyes will appreciate the padding:
